Skip to content

Conversation

@epc-ake
Copy link
Contributor

@epc-ake epc-ake commented Sep 12, 2025

Make sure the driver initializes with the sensors default format.

Copy link
Contributor

@josuah josuah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing, good catch.

Though, by directly asking source_dev, this does not set data->video_format.pitch (responsibility of the receiver).

Using video_get_format() on dev rather than source_dev will make video_esp32_get_fmt() be used, which correctly set the pitch.

Would that make sense? There might be better ways which you may prefer.

Thanks!

Make sure the driver initializes with the sensors default format.

Signed-off-by: Armin Kessler <[email protected]>
@epc-ake epc-ake force-pushed the fix/video_set_default_format branch from cb7a91d to 11f9e57 Compare September 15, 2025 07:07
@epc-ake
Copy link
Contributor Author

epc-ake commented Sep 15, 2025

This was missing, good catch.

Though, by directly asking source_dev, this does not set data->video_format.pitch (responsibility of the receiver).

Using video_get_format() on dev rather than source_dev will make video_esp32_get_fmt() be used, which correctly set the pitch.

Would that make sense? There might be better ways which you may prefer.

Thanks!

Thanks, @josuah! Good catch :-)
Funny enough, I had it right initially but then thought I needed to change it...

Comment on lines +389 to +392
if (!device_is_ready(cfg->source_dev)) {
LOG_ERR("Source device not ready");
return -ENODEV;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to require the sensor to be initialized before the dvp ? If so, you should change the init order of the dvp (currently, they are the same order and no guarantee for that).

Otherwise, I don't see in the code any reason for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the call to video_get_format(dev, &data->video_format) will be forwarded to the source_dev i think this is needed.

The driver expects the source property set and therefore I think the initialization sequence is correct because lcd_cam depends on the sensor driver?
https://github.com/zephyrproject-rtos/zephyr/blob/5bef4a9d6264761c10d739b345eb2f7ded719099/boards/espressif/esp32s3_eye/esp32s3_eye_procpu.dts#L150C1-L150C21

Or should the driver actually be updated to make use of remote-endpoint? In that case yes we would have to handle the initialization sequence differently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, just realize that the esp32_dvp is not yet migrated to use the port / endpoint mechanism, issue here:
#80514

Could you help to do this if possible ? An example is here #88323

With the new remote-endpoint and no direct phandle reference we don't need the sensor initialized before the dvp anymore (if it is not explicitely required for some reason)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I find some time tomorrow. I will do an other PR then.

Comment on lines +394 to +397
if (video_get_format(dev, &data->video_format) < 0) {
LOG_ERR("Failed to get default format from source device");
return -EINVAL;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why the esp32_dvp needs to get the format from the source here in init() function as I don't see it is used after this line.
Basically, format will be set by the application after all drivers initialization and even the sensor doesn't set a format by default, it 's not a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional not to set a default format in the drivers?
In my opinion, sensor drivers should initialize with a default format.
For example, I noticed that with the video shell, the default format before this PR ends up as (fmt=0x0, width=0, height=0), which seems like an invalid state and something that should always be avoided.

Copy link
Contributor

@ngphibang ngphibang Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not intentional, most of the sensor drivers set its default format. However, it is not mandatory, so we should not check this in the receiver init function and return failed as it's not an issue if it is not set at initialization, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josuah could you give your oppinion on this?
Here’s the sequence where I encounter this issue:

  1. Start the Video Shell.
  2. Retrieve the current format and observe that the default format from the sensor driver is already set. This leads to the assumption that setting the same format again is unnecessary.
  3. Attempt to capture images, but receive strange messages indicating that a buffer of size 0 is enqueued.
  4. The system either freezes or responds with an error.

The root cause is that the sensor and capture driver are not properly synchronized after startup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is the assumption that a default format will be set before any intervention from the sample:

/* Get default/native format */
fmt.type = type;
if (video_get_format(video_dev, &fmt)) {
LOG_ERR("Unable to retrieve video format");
return 0;
}

But this is a tricky situation: now users in a might want to modify the driver (where the default comes from) to get the resolution. pixfmt=0x0, width=0, height=0 sounds like sane default: un-configured state, refusing to start with a clear error message (i.e. shell, video protocols stacks, libraries, samples).

In the meantime, setting the default is still the best that can be done?

Copy link
Contributor

@ngphibang ngphibang Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add a check to the samples so that if fmt is not set correctly, we should not continue. The order should be

  • first taking the format from the configs (CONFIG_VIDEO_PIXEL_FORMAT, WIDTH, HEIGHT) and check it then if nothing
  • taking the default format (and check it), then if still nothing, should not continue

Anyway, checking the sensor default format here in the receiver init() and force the driver stop initializing here is not the right way I think, because user can set the format later.

Furthermore, the check :

 if (video_get_format(video_dev, &fmt)) { 
 	LOG_ERR("Unable to retrieve video format"); 
 	return 0; 
 } 

is not sufficient as get_format() can be successful but fmt can be {0}, the check should be done on the valid range (retrieved from caps)

@sonarqubecloud
Copy link

@josuah josuah self-requested a review September 15, 2025 12:46
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Nov 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Video Video subsystem platform: ESP32 Espressif ESP32 Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants